Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Committing TS Firmware Hit Reconstruction Stagger for the Purpose of Triggering Studies #1473

Merged
merged 3 commits into from
Sep 27, 2024

Conversation

rodwyer100
Copy link
Contributor

In my last pull request, I added the validated processor which stagged analysis digi objects to be inserted into our most current version of cluster reconstruction. In this pull request, I am adding the validate processor which inserts the most currect validated version of hit reconstruction into the analysis chain so that Elizabeth may play with it. It is a little different from the S30XL implementation primarily in that its input FIFO pragmas mean that it would need to run around 5 clock cycles as written (though LK said we would likely integrate over 3) and would need 5 copies to run continuously; the S30XL runs continuously and if we were to do that here the pragma structure would need to be altered a little bit.

This is all unnecessary detail, just note that this is the first validated piece of hit making firmware designed for the full LDMX environment that emulates LK's original manner of reconstruction. This is done in TrigScintFirmwareHitProducer, which takes in QIEDigis and outputs TrigScintHit objects. The processor will ultimately be adopted to emulate the NumericalHitProcessor of Andrew and Niramay as that seems the easiest to reproduce in something that can run in firmware. I am including it here because it is the "official" processor I will use to emulate in firmware. If it needs some refashioning or being made pretty, please lmk. I can also include it in a latter pull request alongside the firmware pileUp processor if necessary.

Check List

  • I successfully compiled ldmx-sw with my developments
  • I ran my developments and the following shows that they are successful.

Here are proofs that the firmware emulates the software hits (from QIE adcs and tdcs) faithfully. Note that our ap_int resolution here means that if you fiddle with the gains (i.e. make them bigger) you will note get the right hit PE amplitudes:
SAMPLE OUTPUT:
Analysis barID: 2, PE Number: 59.4437
Analysis barID: 3, PE Number: 97.9569
Analysis barID: 22, PE Number: 79.7444
Analysis barID: 23, PE Number: 89.955
Analysis barID: 35, PE Number: 158.878
Analysis barID: 36, PE Number: 61.3231
DID I GET HERE 3
DID I GET HERE 4
DID I GET HERE 5
Firmware barID: 2, PE Number: 59
Firmware barID: 3, PE Number: 98
Firmware barID: 22, PE Number: 79
Firmware barID: 23, PE Number: 90
Firmware barID: 35, PE Number: 159
Firmware barID: 36, PE Number: 61
Firmware barID: 49, PE Number: 8
DID I GET HERE 6
Note that the reason there is an extra hit printed in the Firmware in bar 49 in this case is because I didn't print analysis hits with amplitude below 30. I can fix that, but I have seen enough of the output to know that the firmware is replicating the software appropriattely.

@tvami
Copy link
Member

tvami commented Sep 20, 2024

@rodwyer100 let me rebase this for you, you have all these commits from the past, I'll push in a second

@rodwyer100
Copy link
Contributor Author

I think the NumericalRecHitProducer may be introducing errors that prevent the build test from working. I am trying to remove it for another commit; its the other stuff I really only need commited on the short term.

@tvami
Copy link
Member

tvami commented Sep 20, 2024

let me rebase this for you

so I tried this, but it leads to way to many conflicts, so I dont wanna touch too much of the code. This is what I did

git fetch
git switch trunk
git pull
git switch -
just format-cpp
git rebase trunk

this will then give you options to pick if you want the new or old

Copy link
Member

@tvami tvami left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some cosmetics comments, but also some real issue with the line ap_uint<14> word6 = FIFO[i][5];. Otherwise cleanup, addition of comments + moving magic numbers to const expressions

TrigScint/src/TrigScint/Firmware/hitproducer_hw.cxx Outdated Show resolved Hide resolved
TrigScint/src/TrigScint/TrigScintFirmwareHitProducer.cxx Outdated Show resolved Hide resolved
TrigScint/src/TrigScint/TrigScintFirmwareHitProducer.cxx Outdated Show resolved Hide resolved
TrigScint/src/TrigScint/TrigScintFirmwareHitProducer.cxx Outdated Show resolved Hide resolved
TrigScint/src/TrigScint/TrigScintFirmwareHitProducer.cxx Outdated Show resolved Hide resolved
TrigScint/src/TrigScint/TrigScintFirmwareHitProducer.cxx Outdated Show resolved Hide resolved
TrigScint/src/TrigScint/TrigScintFirmwareHitProducer.cxx Outdated Show resolved Hide resolved
TrigScint/src/TrigScint/Firmware/hitproducer_hw.cxx Outdated Show resolved Hide resolved
TrigScint/src/TrigScint/TrigScintFirmwareHitProducer.cxx Outdated Show resolved Hide resolved
@tvami
Copy link
Member

tvami commented Sep 21, 2024

but I have seen enough of the output to know that the firmware is replicating the software appropriattely.

I think we should introduce a DQM analyzer that plots these variables, and then we can compare things like what you mention above more easily (I guess this can be done in another PR)

@rodwyer100
Copy link
Contributor Author

Okay so I am trying to pass the build test with warnings interpreted as errors. It seems that there are many warnings unrelated to what I am doing in code thats old. Lmk if you want me to try to fix it, but seeing as its in the HLS primitives used in Trigger I doubt Id be able to.

@tomeichlersmith
Copy link
Member

@rodwyer100 See the changes here: #1470

We can choose to ignore the warnings from HLS but there are several other parts of TS code that can be patched by us.

You can also quicken development by testing the warnings-as-errors compilation locally with

just configure-force-error

before building to update your configuration to treat warnings as errors.

@rodwyer100
Copy link
Contributor Author

Alright everythings been rebased, checked that its building, and fixed w.r.t the changes yall made on the last push

Copy link
Member

@tomeichlersmith tomeichlersmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! I have some questions about how these source files within Firmware are being used outside of ldmx-sw and whether particular declarations and pragma are necessary to keep around.

Besides that, just some cleanup with the logging.

Copy link
Member

@tvami tvami left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! My only request is a follow-up PR that does a DQM analyzer on these developments

@tomeichlersmith tomeichlersmith merged commit 6e4d9f7 into trunk Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants